[ZH] Fix loop index comparison warnings #804

Mauller · 2025-05-03T16:31:29Z

This PR fixes loop index comparison warnings by matching the data type of the loop index to match the comparison values data type.

TODO:

Replicate in Generals.

aliendroid1 · 2025-05-03T16:41:05Z

If you are going to change int to an unsigned type then I think std::size_t would be more appropriate and in line with standard conventions. Where range loops can be used then they should be preferred

xezon

Looks fairly safe, but I suggest to give it a test for mismatch, especially with a focus on USA Countermeasures.

size_t is identical to std::size_t.

xezon · 2025-05-03T16:53:50Z

GeneralsMD/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp

@@ -413,7 +413,7 @@ static void findHighFileNumber( AsciiString filename, void *userData )

 	// strip off the extension at the end of the filename
 	AsciiString nameOnly = filename;
-	for( Int count = 0; count < strlen( SAVE_GAME_EXTENSION ); count++ )
+	for( size_t count = 0; count < strlen( SAVE_GAME_EXTENSION ); count++ )


I wonder if this will call strlen on every iteration...

Yeah i think the VC6 version may do that but modern MSVC would likely compile it out as a constant.

xezon · 2025-05-03T16:57:00Z

GeneralsMD/Code/GameEngine/Source/GameClient/MapUtil.cpp

@@ -579,7 +579,7 @@ Bool MapCache::loadUserMaps()
 		{
 			AsciiString endingStr;
 			AsciiString fname = s+1;
-			for (Int i=0; i<strlen(mapExtension); ++i)
+			for (size_t i=0; i<strlen(mapExtension); ++i)


Another strlen in condition...

yeah... i didn't want to change too much.

xezon · 2025-05-03T16:58:09Z

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Behavior/CountermeasuresBehavior.cpp

@@ -301,7 +301,7 @@ void CountermeasuresBehavior::launchVolley()
 	Object *obj = getObject();

 	Real volleySize = (Real)data->m_volleySize;
-	for( int i = 0; i < data->m_volleySize; i++ )
+	for( UnsignedInt i = 0; i < data->m_volleySize; i++ )


I hope this will produce the same float value below.

In theory it should as the raw binary values of positive signed int and unsigned int values are the same up to 2.7 billion at least.

This changes 34% of assembler in CountermeasuresBehaviour::launchVolley.

interesting how it changes that much when the index is only used in one place.

GeneralsMD/Code/GameEngine/Source/GameNetwork/ConnectionManager.cpp

GeneralsMD/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp

Mauller · 2025-05-04T10:05:29Z

Updated this with the inclusion of a cast that was missing in one of the loops.

Mauller · 2025-05-04T16:41:16Z

Just rebased off recent main after the other warning fixes were merged.

Mauller · 2025-05-04T18:34:22Z

Just ran Golden Replay 1 against this with the VC6 build and it did not mismatch.
Not sure if countermeasures were explicitly used by any of the USA players. But they were using every vehicles in the game during the test.

So I think the changes are fairly safe, i can replicate the changes to generals if people are happy.

xezon

unassemblize shows significant assembler changes in the following 4 functions. I suggest to focus mismatch test on functionality related to these.

ScriptActions::doMoveTeamTowardsNearest
DeliverPayloadNugget::create
W3DLaserDraw::W3DLaserDraw
CountermeasuresBehaviour::launchVolley

xezon · 2025-05-08T18:06:09Z

GeneralsMD/Code/GameEngine/Source/GameLogic/ScriptEngine/ScriptActions.cpp

@@ -5296,7 +5296,7 @@ void ScriptActions::doMoveTeamTowardsNearest( const AsciiString& teamName, const
 		{
 			Real closestDist;
 			Real dist;
-			for( Int typeIndex = 0; typeIndex < objectTypes->getListSize(); typeIndex++ )
+			for( size_t typeIndex = 0; typeIndex < objectTypes->getListSize(); typeIndex++ )


This creates a strange diff in ScriptActions::doMoveTeamTowardsNearest

yeah i don't get that, if anything i would have through that most code changes would be reduction in code due to not needing to perform some casts.

It looks as if the function call has changed, but perhaps it is just an error in the tool.

xezon · 2025-05-08T18:12:58Z

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/ObjectCreationList.cpp

@@ -322,7 +322,7 @@ class DeliverPayloadNugget : public ObjectCreationNugget
 		}

 		Object *firstTransport = NULL;
-		for( Int formationIndex = 0; formationIndex < m_formationSize; formationIndex++ )
+		for( UnsignedInt formationIndex = 0; formationIndex < m_formationSize; formationIndex++ )


This innocent looking edit changes 13% of the assembler inside DeliverPayloadNugget::create.

xezon · 2025-05-08T18:14:54Z

GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DLaserDraw.cpp

@@ -156,7 +156,7 @@ W3DLaserDraw::W3DLaserDraw( Thing *thing, const ModuleData* moduleData ) :
 	//Allocate an array of lines equal to the number of beams * segments
 	m_line3D = NEW SegmentedLineClass *[ data->m_numBeams * data->m_segments ];

-	for( int segment = 0; segment < data->m_segments; segment++ )
+	for( UnsignedInt segment = 0; segment < data->m_segments; segment++ )


This changes 55% of the assembler in W3DLaserDraw::W3DLaserDraw.

thats interesting, you would think it would just be a few casts that got changed since segment is only used it one place within the function

xezon · 2025-05-08T18:22:06Z

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Behavior/CountermeasuresBehavior.cpp

@@ -301,7 +301,7 @@ void CountermeasuresBehavior::launchVolley()
 	Object *obj = getObject();

 	Real volleySize = (Real)data->m_volleySize;
-	for( int i = 0; i < data->m_volleySize; i++ )
+	for( UnsignedInt i = 0; i < data->m_volleySize; i++ )


This changes 34% of assembler in CountermeasuresBehaviour::launchVolley.

Mauller force-pushed the fix-loop-index-warnings branch from 14f7171 to 8aa4016 Compare May 3, 2025 16:32

Mauller marked this pull request as ready for review May 3, 2025 16:39

Mauller self-assigned this May 3, 2025

Mauller added Minor Severity: Minor < Major < Critical < Blocker Build Anything related to building, compiling ZeroHour Relates to Zero Hour labels May 3, 2025

xezon approved these changes May 3, 2025

View reviewed changes

aliendroid1 reviewed May 3, 2025

View reviewed changes

GeneralsMD/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp Show resolved Hide resolved

Mauller force-pushed the fix-loop-index-warnings branch from 8aa4016 to f4c5b85 Compare May 4, 2025 07:26

Mauller added 2 commits May 4, 2025 17:40

[ZH] Fix loop index type for container size comparison warnings (#804)

68ad9f9

[ZH] Fix loop index type for unsigned integer comparison warnings (#804)

c0eff59

Mauller force-pushed the fix-loop-index-warnings branch from f4c5b85 to c0eff59 Compare May 4, 2025 16:40

xezon reviewed May 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ZH] Fix loop index comparison warnings #804

[ZH] Fix loop index comparison warnings #804

Mauller commented May 3, 2025 •

edited

Loading

aliendroid1 commented May 3, 2025

xezon left a comment •

edited

Loading

xezon May 3, 2025

Mauller May 3, 2025

xezon May 3, 2025

Mauller May 3, 2025

xezon May 3, 2025

Mauller May 4, 2025

xezon May 8, 2025

Mauller May 8, 2025

Mauller commented May 4, 2025

Mauller commented May 4, 2025

Mauller commented May 4, 2025

xezon left a comment

xezon May 8, 2025

Mauller May 8, 2025

xezon May 8, 2025

xezon May 8, 2025

xezon May 8, 2025

Mauller May 8, 2025

xezon May 8, 2025

[ZH] Fix loop index comparison warnings #804

Are you sure you want to change the base?

[ZH] Fix loop index comparison warnings #804

Conversation

Mauller commented May 3, 2025 • edited Loading

aliendroid1 commented May 3, 2025

xezon left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mauller commented May 4, 2025

Mauller commented May 4, 2025

Mauller commented May 4, 2025

xezon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mauller commented May 3, 2025 •

edited

Loading

xezon left a comment •

edited

Loading